Add clarification about using destroy with pointers to value types#2112
Add clarification about using destroy with pointers to value types#2112JinShil wants to merge 1 commit into
Conversation
|
Thanks for your pull request, @JinShil! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
|
Or do we need an overload...something like this? void destroy(T)(T* obj)
{
destroy(*obj);
} |
schveiguy
changed the title
| /******** | ||
| To destroy a value type through a pointer, the pointer must be dereferenced. | ||
| Otherwise `destroy` will only destroy the pointer itself, not the entity being | ||
| pointed to. |
| */ | ||
| unittest | ||
| { | ||
| struct S |
| ~this() { dtorCount++; } | ||
| } | ||
|
|
||
| void main() |
There was a problem hiding this comment.
Leave this out, it's a unit test. Otherwise, the test doesn't actually get run.
| Otherwise `destroy` will only destroy the pointer itself, not the entity being | ||
| pointed to. | ||
| */ | ||
| unittest |
There was a problem hiding this comment.
@safe or @system? Not sure which one applies, but it needs to be tagged.
| assert(S.dtorCount == 0); | ||
|
|
||
| S* s2 = new S(); | ||
| destroy(*s2); // destroys the struct pointed to by `s2` |
| void main() | ||
| { | ||
| S* s1 = new S(); | ||
| destroy(s1); // destroys only the pointer `s1`, not what it points to. |
There was a problem hiding this comment.
clarify that destroying a pointer simply just sets it to null.
If we can verify that this isn't going to interfere with other things like recursive destruction, I'd be all for this. It's what people would expect I would think. |
PetarKirov
left a comment
PetarKirov
left a comment
There was a problem hiding this comment.
The changes LGTM, though I wonder if would be better to do the dereferencing automatically (probably by means of another overload).
@ZombineDev see my last comment in this PR. I added such an overload in PR #2115 |
|
After the discussion in #2115, it became apparent that there are a few problems with the current implementation of Depending on how that goes, this PR will either not be needed, or will need modification. For now, there's no use for it being in the PR queue consuming resources, until the implementation of |
|
There are no resources consumed and its easy to lose track of work if it's closed, so I strongly suggest keeping this open until it's really not needed anymore. Just set Blocked as title or label and no one will look at it casually. |
5 participants
See discussion here (dlang/dmd#7947 (comment)) for context.
Also see http://ddili.org/ders/d.en/memory.html#ix_memory.destroy
cc @schveiguy @wilzbach